Skip to content

Add ws_log_format message directive #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Apr 21, 2021
Merged

Add ws_log_format message directive #3

merged 4 commits into from
Apr 21, 2021

Conversation

@zaslavskii
Copy link

@zaslavskii zaslavskii commented Apr 16, 2021 •
edited
Loading

  1. Fixed bug when global variables remain dirty even though the nginx configuration is reloaded
  2. Fixed a bug, when endpoint stops working if some directives are not filled: made directives optional
  3. Log file defaults to current error_log file
  4. Add ws_log_format "message" directive to log full messages, not chunks. Logged all non-control frames (https://tools.ietf.org/html/rfc6455#section-5.2).
  5. Add ws_log_enabled to disable/enable module for the server block
  6. Add ws_message_size template variable
  7. Replaced global variables with srv configuration (http://nginx.org/en/docs/dev/development_guide.html#http_conf)

Some issues:

  1. number of bytes sent when ws conn is opened is not logged. It's quite small and includes only headers.
  2. The number of bytes logged only refer to the size of the payload:
USERMETRICS nd-123-456 rg-123-456 full ws_open 0 0 200 dedicated
FRAME 143 # this is what was sent - 143 bytes ({"jsonrpc": "2.0", "method":"eth_getBlockByHash","params":["0xb49c80c7d55fde1c7bb7385169c972442077f5fa9c5767332f83589e5ddb4ae8", true],"id": 1})
FRAME 170040 # this is what was received as a response (the full output will be quite heavy, this is a 170040 char json that ends with \n\n
FRAME 0
172.17.0.1 - - [20/Apr/2021:12:03:49 +0000] "GET / HTTP/1.1" 101 170050 223"-" "-" "-"

# 170050 223 - this is what nginx logs by default when ws is closed. I suppose it also includes headers and some non payload  data. I can spend some time understanding where this difference comes from, but I don't think if it worths it.

Example configuration

  server {
    listen 0.0.0.0:80;
    ws_log_enabled on;
    ws_log_format message "USERMETRICS nd-123-456 rg-123-456 full ws_open $ws_message_size 0 200 dedicated";
    ws_log_format open "USERMETRICS nd-123-456 rg-123-456 full ws_open 0 0 200 dedicated"; # or we can skip it completely, as we care only about the real payload

Links:
https://tools.ietf.org/html/rfc6455#section-5.2
http://nginx.org/en/docs/dev/development_guide.html#http_conf

@zaslavskii zaslavskii force-pushed the feature/add-count-messages-not-frames branch 9 times, most recently from 87ef992 to e6e5a3a Compare April 16, 2021 19:24
@zaslavskii zaslavskii changed the title Add ws_log_format message directive WIP Add ws_log_format message directive Apr 16, 2021
@zaslavskii zaslavskii requested a review from easeev April 19, 2021 05:33
@zaslavskii zaslavskii force-pushed the feature/add-count-messages-not-frames branch 3 times, most recently from b2b2c08 to 3a9799c Compare April 20, 2021 11:56
@zaslavskii
Add srv configuration structure
543c013 
Add message template
@zaslavskii zaslavskii force-pushed the feature/add-count-messages-not-frames branch from 3a9799c to 543c013 Compare April 20, 2021 11:56
@zaslavskii zaslavskii changed the title WIP Add ws_log_format message directive Add ws_log_format message directive Apr 20, 2021
@zaslavskii
Revert ability to build module statically
2cb73ba 
Signed-off-by: Anton Zaslavskiy <anton.zaslavskiy@chainstack.com>
fan-tom
fan-tom reviewed Apr 20, 2021
View reviewed changes
fan-tom
fan-tom reviewed Apr 20, 2021
View reviewed changes
fan-tom
fan-tom reviewed Apr 20, 2021
View reviewed changes
fan-tom
fan-tom reviewed Apr 20, 2021
View reviewed changes
ngx_http_websocket_stat_module.c Outdated
{
template_ctx_s *ctx = data;
ngx_frame_counter_t *frame_cntr = &ctx->ws_ctx->frame_counter;
if (!ctx || !frame_cntr)
Copy link

@fan-tom fan-tom Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UB: you already dereferenced ctx, so it cannot be null here

Copy link
Author

@zaslavskii zaslavskii Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that! I will change the condition as well as specify types of the arguments explicitly

UPD: can't change signatures of the functions as it will require significant refactoring

fan-tom
fan-tom reviewed Apr 20, 2021
View reviewed changes
fan-tom
fan-tom reviewed Apr 20, 2021
View reviewed changes
ngx_http_websocket_stat_module.c Outdated
ngx_http_websocket_srv_conf_t *srvcf;

srvcf = ngx_pcalloc(cf->pool, sizeof(ngx_http_websocket_srv_conf_t));
if (srvcf == NULL) {
Copy link

@fan-tom fan-tom Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier pointers were checked for null by simple negation. Don't you want to be consistent?

fan-tom
fan-tom reviewed Apr 20, 2021
View reviewed changes
ngx_http_websocket_stat_module.c Outdated
Comment on lines 635 to 643
if (conf->enabled && !conf->ws_log) {
init_ws_log_file(cf, conf, &cf->cycle->error_log);
}

if (conf->enabled && !conf->ws_log)
return NGX_CONF_ERROR;

if (conf->enabled && !conf->ws_log->file)
return NGX_CONF_ERROR;
Copy link

@fan-tom fan-tom Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (conf->enabled && !conf->ws_log) {
init_ws_log_file(cf, conf, &cf->cycle->error_log);
}
if (conf->enabled && !conf->ws_log)
return NGX_CONF_ERROR;
if (conf->enabled && !conf->ws_log->file)
return NGX_CONF_ERROR;
if (conf->enabled && !conf->ws_log) {
init_ws_log_file(cf, conf, &cf->cycle->error_log);
if (!conf->ws_log || !conf->ws_log->file)
return NGX_CONF_ERROR;
}

@zaslavskii
Update README according to the directives changes
a08eee5 
Signed-off-by: Anton Zaslavskiy <anton.zaslavskiy@chainstack.com>
@zaslavskii zaslavskii force-pushed the feature/add-count-messages-not-frames branch from 30eae27 to a08eee5 Compare April 20, 2021 17:23
@zaslavskii @fan-tom
Apply suggestions from code review
0580fab 
Co-authored-by: Alexey Gerasimov <lokomot476@gmail.com>
@zaslavskii zaslavskii merged commit a45e146 into master Apr 21, 2021
@zaslavskii zaslavskii deleted the feature/add-count-messages-not-frames branch April 21, 2021 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@easeev easeev Awaiting requested review from easeev

@RostislavKonstantinov RostislavKonstantinov Awaiting requested review from RostislavKonstantinov

@kutysam kutysam Awaiting requested review from kutysam

1 more reviewer

@fan-tom fan-tom fan-tom left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@zaslavskii @fan-tom